Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: only build _ext and kernels #2813

Merged
merged 4 commits into from
Nov 13, 2023
Merged

chore: only build _ext and kernels #2813

merged 4 commits into from
Nov 13, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Nov 10, 2023 •
edited
Loading

Right now, we install three libraries: _ext, libawkward, libawkward-cpu-kernels, and build some additional static variants.

We didn't change this when splitting out awkward-cpp, because I didn't want to rock the boat any more. Since becoming more well versed with CMake's nuances, I'm confident that we can remove the leftover baggage.

This PR keeps the separate libawkward and _ext libraries (because it doesn't appear trivial to merge them), but drops the static libraries.

@codecov Codecov
Copy link

codecov bot commented Nov 10, 2023 •
edited
Loading

Codecov Report

Merging #2813 (05ee4a2) into main (9d874a0) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@agoose77
chore: remove object/static library targets
1c1cb8b 
We now export some more symbols to support this
@agoose77 agoose77 force-pushed the agoose77/chore-build-single-lib branch from 0636065 to 1c1cb8b Compare November 10, 2023 14:24
@agoose77 agoose77 force-pushed the agoose77/chore-build-single-lib branch from b463e10 to 7faabab Compare November 10, 2023 21:30
@agoose77
Copy link
Collaborator Author

@jpivarski this PR fails on Windows. The error message in the CI indicates that the copy-assignment constructor for GrowableBuffer is being called by std::vector, but this constructor is implicitly deleted if one defines a move-assignment constructor.

I'm not sure why it's suddenly an issue. It's possible that the CI toolchain bumped MSVC at the same time that I wrote this PR, but I doubt it; in this PR I also added an EXPORT_SYMBOL for the FromJsonObjectSchema class, which has std::vector<GrowableBuffer<T>> members. That seems like causative.

This raises the following questions for me:

  1. Why does GCC not complain if MSVC does?
  2. Why does the standard not imply that the V in std::vector<V> needs to be copy-assignable (and copy-constructible?)? It used to in C++11, but seems to relax the rule for C++17+ (see "Template parameters" in https://en.cppreference.com/w/cpp/container/vector).
  3. Is it reasonable to add a copy-assignment constructor to GrowableBuffer? It seems to me like we really don't want that.

I'll keep digging.

@agoose77
Copy link
Collaborator Author

Well, that (05ee4a2) worked! I'm not entirely clear on why this became an error once I exported this class, but it seems that the emission of the default copy-assignment constructor leads to the attempted copy-assignment of the GrowableBuffer.

Clearly, we don't want that, so 05ee4a2 deletes the top-level copy-assignment (rather than implementing it)

@agoose77 agoose77 force-pushed the agoose77/chore-build-single-lib branch from a6ea59b to 05ee4a2 Compare November 10, 2023 23:07
jpivarski
jpivarski approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reducing the number of shared libraries to two (one is the Python module and the other is dlopened through ctypes) is ideal.

However, this PR doesn't change awkward-cpp/src/awkward_cpp/cpu_kernels.py to look for kernels in libawkward.so. It's odd that it passes tests: how does the kernel dispatch find the CPU kernels?

Are the tests picking up a cached version of awkward-cpp?

awkward-cpp/include/awkward/io/json.h Show resolved Hide resolved
@agoose77 agoose77 merged commit 3bb4367 into main Nov 13, 2023
66 of 71 checks passed
@agoose77 agoose77 deleted the agoose77/chore-build-single-lib branch November 13, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpivarski jpivarski jpivarski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@agoose77 @jpivarski